Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metricbeat enterprise search module: add xpack.enabled support #29871

Merged

Conversation

carlosdelest
Copy link
Member

@carlosdelest carlosdelest commented Jan 17, 2022

What does this PR do?

Add xpack.enabled support for Enterprise Search Metricbeat module.

This allows metrics retrieved to be available in Stack Monitoring Kibana plugin, as this plugin changes the indices retrieved from .metricbeat-* to .monitoring-*. xpack.enabled: true allows changing the index to be used to .monitoring-*, as already implemented for Elasticsearch, Kibana and Logstash modules.

Why is it important?

This change is needed to allow Enterprise Search Metricbeat module to produce metrics that can be consumed by Stack Monitoring Kibana plugin. Stack Monitoring won't display Enterprise Search metrics otherwise.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 17, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 17, 2022

This pull request does not have a backport label. Could you fix it @carlosdelest? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 17, 2022
@carlosdelest carlosdelest added the backport-v8.0.0 Automated backport with mergify label Jan 17, 2022
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Jan 17, 2022
@carlosdelest carlosdelest added backport-v8.1.0 Automated backport with mergify enhancement labels Jan 17, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 17, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 17, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-18T11:09:49.356+0000

  • Duration: 101 min 14 sec

  • Commit: 1d70a33

Test stats 🧪

Test Results
Failed 0
Passed 9722
Skipped 2530
Total 12252

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@carlosdelest carlosdelest added Team:Integrations Label for the Integrations team and removed Team:Observability labels Jan 18, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@ruflin
Copy link
Contributor

ruflin commented Jan 20, 2022

@carlosdelest Is enterprise search already today with a different mechanism shipping metrics to the .monitoring index? If no, I wonder why we need this? @jasonrhodes I would assume stack monitoring can pick up data from any index assuming the user permissions match?

@carlosdelest carlosdelest removed the backport-v8.0.0 Automated backport with mergify label Jan 20, 2022
@carlosdelest
Copy link
Member Author

Hi @ruflin :

Enterprise Search can't use Metricbeat 8.0 to ship monitoring information, as Metricbeat no longer supports ILM settings that allows shipping to .monitoring-* indices using Elasticsearch output configuration.

Enterprise Search 8.0 will be using a bundled 7.16 Metricbeat version so it can ship monitoring information using an ILM-backed .monitoring-* index.

This is temporary, as the final picture would be for Enterprise Search to use Metricbeat 8.x with xpack.enabled support (this PR), plus Elasticsearch index templates for creating the data streams once they are sent (elastic/elasticsearch#82743).

Stack Monitoring will be able to gather Enterprise Search monitoring as long as it is sent to .monitoring-ent-search-* indices, which both approaches provide. We're just trying to align on the monitoring approach used by other solutions (Elasticsearch, Kibana, Logstash).

Happy to discuss and consider alternatives!

@jasonrhodes
Copy link
Member

@ruflin this surprises me as well. I assumed that since this is a new metricbeat module, there's no reason to maintain any xpack.enabled fork. I'm re-reading the conversation over here: elastic/kibana#121975 (comment)

My question for @carlosdelest (and possibly for @sayden / @klacabane) is: does the enterprise search MB module need to be able to write to metricbeat-* for any reason going forward? If not, can we at the very least hardcode xpack.enabled: true as the only way for this module to run, even if that flag technically needs to be set for MB internals to write the data to the correct indices?

@carlosdelest
Copy link
Member Author

does the enterprise search MB module need to be able to write to metricbeat-* for any reason going forward?

Not that I can think of, unless we wanted to use Metricbeat 8.x with a Kibana + Enterprise Search 7.16.

I agree on hardcoding xpack.enabled. Would you like this to be the default behaviour, and be disabled explicitly in configuration if it is ever needed? Or always use the new behaviour?

Is this the way to go as well for the rest of the solutions, so we are aligned with them?

Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this today along with the ES change and this metricbeat config:

  - module: enterprisesearch
    xpack.enabled: true
    period: 10s
    hosts: ["http://localhost:3002"]
    username: elastic
    password: changeme

Looks good!

Screen Shot 2022-01-24 at 15 23 27

Screen Shot 2022-01-24 at 15 23 41

@@ -24,7 +24,7 @@ services:
- 3002:3002

elasticsearch:
image: docker.elastic.co/integrations-ci/beats-elasticsearch:${ELASTICSEARCH_VERSION:-7.15.0}-1
image: docker.elastic.co/integrations-ci/beats-elasticsearch:${ELASTICSEARCH_VERSION:-8.0.0-SNAPSHOT}-1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be 8.1? I'm still not totally clear on where these compose files factor into the release pipeline, so feel free to ignore if I'm wrong.


- module: enterprisesearch
xpack.enabled: true
metricsets: ["health", "stats"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the metricsets? The elasticsearch module specifies a locked metricset when running in xpack mode

xpackEnabledMetricSets := []string{

Seems to work find w/o the list in my testing. Maybe because it just enables all of them.

@ruflin
Copy link
Contributor

ruflin commented Jan 24, 2022

The part I'm still missing, is there anything else already today writing to .monitoring-* for enterprise search? If not, couldn't we jump directly to the "end" story in this scenario which is the logs-*-* and metrics-*-* data streams? https://www.elastic.co/blog/an-introduction-to-the-elastic-data-stream-naming-scheme

@carlosdelest
Copy link
Member Author

The part I'm still missing, is there anything else already today writing to .monitoring-* for enterprise search?

I believe not.

If not, couldn't we jump directly to the "end" story in this scenario which is the logs-- and metrics-- data streams?

@ruflin , I agree on hardcoding xpack.enabled. Would you like this to be the default behaviour, and be disabled explicitly in configuration if it is ever needed? Or always use the new behaviour and remove xpack.enabled?

Is this the way to go as well for the rest of the solutions, so we are aligned with them?

@ruflin
Copy link
Contributor

ruflin commented Jan 24, 2022

It might be a bit unexpected from a user perspective that metricbeat has a module which cannot ship to metricbeat-* indices but at the same time if there are no dashboards and the data is only for internal use why support it in the first place.

For all the other stack monitoring modules, a discussion is ongoing on how we get to metrics-*-* indices but there is a catch because there is also internal data shipping in ES and Kibana. But for enterprise search it sounds like we don't have this limitation. So instead of shipping to .monitoring-* we ship directly to metrics-*-*. This would allow in theory users to also build dashboards and have the setup at one stage rely on packages But for now also an internal template in Elasticsearch can be used how we would do it for .monitoring indices.

My understanding of hardcoding xpack.enabled is more that it doesn't exist in the first place as there is only one way to ship the data. But I think we talk about the same thing.

@jasonrhodes Does this align with your long term plan for stack monitoring?

@carlosdelest
Copy link
Member Author

pinging @ravikesarwani for comments . Thanks!

@ravikesarwani
Copy link

I will let @jasonrhodes weigh in on this as well but as I understand the current UI code (8.0) doesn't read from metrics-*.
We are working on it but it's targeted for 8.1. Making all the metricbeat modules work the same way is a big win from the customer understanding perspective and avoids a lot of confusion (like retention management and so on considering both self-managed and cloud environments) as metricbeat method will exist for a little while.

The Agent package will write to metrics-* and provide backward compatibility from UI perspective and that's where we are heading for all stack packages including Enterprise search. This is where we will focus on adding default dashboards etc. as well. There's already quite a bit of complexities and IMO making one metricbeat module work completely different will just to the confusion from users perspective.

@matschaffer
Copy link
Contributor

matschaffer commented Jan 25, 2022

Basic support for metrics-*-* just landed in elastic/kibana#119112 for 8.1 (cc @neptunian), but I understand we haven't done comprehensive testing of using the new index patterns.

They're present and we've validated that the changes don't break the .monitoring-* pattern used by other metricbeat stack monitoring modules.

It might just work, but I think we should still merge this change to minimize the number of ent search releases that might land documents in the no-longer-queried metricbeat-* pattern.

Then we can work on a comprehensive change from .monitoring-* to metrics-* for all stack components.

@carlosdelest
Copy link
Member Author

Thanks for all your feedback on this issue!

Re xpack.enabled behaviour, I think it's better from a user / compatibility perspective not to break the 7.x behaviour, shipping to .metricbeat-* indices. It also aligns with other current Stack Monitoring behaviour and configuration for other modules.

I'll let the PR as is, and I'll be merging this tomorrow unless some objection is raised.

@ruflin
Copy link
Contributor

ruflin commented Jan 26, 2022

.metricbeat-* indices? I assume you mean .monitoring-*? Just double checking.

@carlosdelest
Copy link
Member Author

carlosdelest commented Jan 26, 2022

.metricbeat-* indices? I assume you mean .monitoring-*? Just double checking.

  • With xpack.enabled: false (the default behaviour in case it is not configured), it will ship to metricbeat-* indices (same behaviour as 7.x for Enterprise Search module)
  • With xpack.enabled: true (change introduced in this PR), it will ship to .monitoring-* indices. Needs explicit configuration.

@ruflin
Copy link
Contributor

ruflin commented Jan 26, 2022

I'm stumbling over the . in front of metricbeat. This is not something use anywhere AFAIK.

@carlosdelest
Copy link
Member Author

carlosdelest commented Jan 26, 2022

I'm stumbling over the . in front of metricbeat. This is not something use anywhere AFAIK.

My bad 😅 . It's metricbeat-* and not .metricbeat-*. Edited the previous comment, thanks for double checking!

@carlosdelest carlosdelest merged commit 2b99db9 into master Jan 27, 2022
v1v added a commit to v1v/beats that referenced this pull request Jan 31, 2022
…k-version-after-8-0-creation

* upstream/master: (69 commits)
  Update stale config following (elastic#30082)
  Make include_matches backwards compatible with 7.x config (elastic#30032)
  [Filebeat] Update handling of elasticsearch server logs (elastic#30018)
  Remove SSL3 support from libbeat and its documentation. (elastic#30071)
  Revert "Packaging: rename arm64 suffix to aarch64 in the tar.gz artifacts ONLY (elastic#28813)" (elastic#30083)
  [libbeat] Add script processor to all beats (elastic#29752)
  Add fonts to support more different types of characters for multiple languages (elastic#29861)
  libbeat/reader: Fix messge conversion to beat.Event (elastic#30057)
  probot[stale]: ignore issues with the tag flaky-test (elastic#30065)
  [DOCS] Add redirect for GSuite module (elastic#30034)
  [Automation] Update elastic stack version to 8.1.0-aa69d697 for testing (elastic#30012)
  Remove msitools install for windows build, using the latest docker image with msitools preinstalled (elastic#30040)
  filebeat/generator/fields: fix dropped error (elastic#29943)
  Include the error message with auditd module events (elastic#30009)
  [Metricbeat] gcp: add firestore metricset (elastic#29918)
  probot: update stale dates (elastic#29997)
  Metricbeat enterprise search module: add xpack.enabled support (elastic#29871)
  x-pack/packetbeat: install Npcap at start-up when required (elastic#29112)
  [Filebeat] Fix panic in decode_cef when recovering from invalid data (elastic#30038)
  Correctly fixe how selected packages are defined (elastic#30039)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.1.0 Automated backport with mergify enhancement Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants